Skip to content

Conversation

@magnetised
Copy link
Contributor

Fixes #31

that emits sync events from updates within a sandboxed transaction
@magnetised magnetised requested a review from thruflo July 30, 2025 16:10
@thruflo
Copy link
Contributor

thruflo commented Jul 30, 2025

I've followed the instructions. I get:

➜  burn git:(main) ✗ mix test
** (exit) exited in: GenServer.call(Phoenix.Sync.Sandbox.StackRegistry, {:lookup, #PID<0.276.0>}, 5000)
    ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
    (elixir 1.18.1) lib/gen_server.ex:1121: GenServer.call/3
    (phoenix_sync 0.4.4) lib/phoenix/sync/sandbox.ex:342: anonymous fn/1 in Phoenix.Sync.Sandbox.lookup_stack_id/1
    (elixir 1.18.1) lib/stream.ex:520: anonymous fn/3 in Stream.flat_map/2
    (elixir 1.18.1) lib/stream.ex:1004: Stream.do_transform_user/6
    (elixir 1.18.1) lib/enum.ex:1166: Enum.find/3
    (phoenix_sync 0.4.4) lib/phoenix/sync/sandbox/postgres/adapter.ex:291: Phoenix.Sync.Sandbox.Postgres.Adapter.insert/6
    (ecto 3.13.2) lib/ecto/repo/schema.ex:1000: Ecto.Repo.Schema.apply/4
    (ecto 3.13.2) lib/ecto/repo/schema.ex:500: anonymous fn/15 in Ecto.Repo.Schema.do_insert/4

My config/test.exs contains:

config :phoenix_sync,
  env: :test,
  mode: :sandbox,
  repo: Burn.Repo

My test_helper.exs contains:

ExUnit.start()
Ecto.Adapters.SQL.Sandbox.mode(Burn.Repo, :manual)

My data case:

defmodule Burn.DataCase do
  use ExUnit.CaseTemplate

  using do
    quote do
      alias Burn.Repo

      import Ecto
      import Ecto.Changeset
      import Ecto.Query
      import Burn.DataCase
    end
  end

  setup tags do
    Burn.DataCase.setup_sandbox(tags)
    :ok
  end

  @doc """
  Sets up the sandbox based on the test tags.
  """
  def setup_sandbox(tags) do
    pid = Ecto.Adapters.SQL.Sandbox.start_owner!(Burn.Repo, shared: not tags[:async])
    on_exit(fn -> Ecto.Adapters.SQL.Sandbox.stop_owner(pid) end)

    # Start sandbox replication stack.
    Phoenix.Sync.Sandbox.start!(Burn.Repo, pid, shared: not tags[:async])
  end
end

I guess I need to start the Sandbox.StackRegistry but I would assume that Phoenix.Sync.Sandbox.start!(...) does that?

@magnetised
Copy link
Contributor Author

I guess I need to start the Sandbox.StackRegistry but I would assume that Phoenix.Sync.Sandbox.start!(...) does that?

No, it should be started as part of the application. I'm sure I hit this exact problem but damned if I can remember the cause. I fixed it but obviously failed to capture the solution in the docs. Might be your version of electric... Do you have electric as a dep? That would prevent the sandbox supervisor from starting

@thruflo
Copy link
Contributor

thruflo commented Jul 31, 2025

Pulled latest branch and avoided starting shape subscriptions before sandbox and my tests run.

Next issue is that my code writes a map to a JSONB field and this raises a protocol String.Chars not implemented for type Map error:

15:31:24.973 [error] GenServer {Phoenix.Sync.Sandbox.Registry, {Phoenix.Sync.Sandbox.Producer, "Phoenix.Sync.Sandbox.Stack-576460752303411963"}} terminating
** (Protocol.UndefinedError) protocol String.Chars not implemented for type Map. This protocol is implemented for the following type(s): Atom, BitString, Date, DateTime, Decimal, Electric.Postgres.Lsn, Electric.Replication.LogOffset, Float, Floki.Selector, Floki.Selector.AttributeSelector, Floki.Selector.Combinator, Floki.Selector.Functional, Floki.Selector.PseudoClass, Integer, List, NaiveDateTime, Phoenix.LiveComponent.CID, Postgrex.Copy, Postgrex.Query, RemoteIp.Block, Time, URI, Version, Version.Requirement

Got value:

    %{"text" => "Lorem ipsum"}

    (elixir 1.18.1) lib/string/chars.ex:3: String.Chars.impl_for!/1
    (elixir 1.18.1) lib/string/chars.ex:22: String.Chars.to_string/1
    (phoenix_sync 0.4.4) lib/phoenix/sync/sandbox/producer.ex:118: anonymous fn/1 in Phoenix.Sync.Sandbox.Producer.record/1
    (elixir 1.18.1) lib/enum.ex:1714: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.18.1) lib/enum.ex:1714: Enum."-map/2-lists^map/1-1-"/2
    (elixir 1.18.1) lib/map.ex:262: Map.new_from_enum/2
    (phoenix_sync 0.4.4) lib/phoenix/sync/sandbox/producer.ex:79: Phoenix.Sync.Sandbox.Producer.msg_from_change/3
    (elixir 1.18.1) lib/enum.ex:1840: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
    (phoenix_sync 0.4.4) lib/phoenix/sync/sandbox/producer.ex:53: Phoenix.Sync.Sandbox.Producer.handle_cast/2
    (stdlib 6.2) gen_server.erl:2371: :gen_server.try_handle_cast/3
    (stdlib 6.2) gen_server.erl:2433: :gen_server.handle_msg/6
    (stdlib 6.2) proc_lib.erl:329: :proc_lib.init_p_do_apply/3
Last message: {:"$gen_cast", {:emit_changes, [{:insert, %{context: nil, prefix: nil, source: "events", autogenerate_id: {:id, :id, :binary_id}, schema: Burn.Threads.Event}, [id: <<167, 101, 138, 87, 219, 173, 67, 78, 172, 253, 62, 251, 212, 11, 31, 203>>, type: "text", data: %{"text" => "Lorem ipsum"}, thread_id: <<72, 40, 246, 67, 68, 230, 64, 178, 183, 236, 192, 237, 36, 40, 241, 62>>, user_id: <<228, 130, 101, 156, 201, 183, 66, 107, 147, 255, 93, 175, 24, 142, 206, 34>>, inserted_at: ~N[2025-07-31 22:31:24], updated_at: ~N[2025-07-31 22:31:24]]}]}}

@thruflo
Copy link
Contributor

thruflo commented Jul 31, 2025

On the "don't start shape subscriptions until the sandbox is ready" front, this is my hacked application.ex:

defmodule Burn.Application do
  use Application

  @impl true
  def start(_type, _args) do
    children = [
      BurnWeb.Telemetry,
      Burn.Repo,
      {DNSCluster, query: Application.get_env(:burn, :dns_cluster_query) || :ignore},
      {Phoenix.PubSub, name: Burn.PubSub},
      {Finch, name: Burn.Finch},
      {Registry, keys: :unique, name: Burn.Agents},
    ] ++ sync_spawning_children(Mix.env()) ++ [
      {BurnWeb.Endpoint, phoenix_sync: Phoenix.Sync.plug_opts()}
    ]

    opts = [strategy: :one_for_one, name: Burn.Supervisor]
    Supervisor.start_link(children, opts)
  end

  defp sync_spawning_children(:test), do: []
  defp sync_spawning_children(_), do: [Burn.Agents.Supervisor]

  @impl true
  def config_change(changed, _new, removed) do
    BurnWeb.Endpoint.config_change(changed, removed)

    :ok
  end
end

Eyeballing it, I wonder if there could be an API like:

defmodule Burn.Application do
  use Application

  @impl true
  def start(_type, _args) do
    children = [
      BurnWeb.Telemetry,
      Burn.Repo,
      {DNSCluster, query: Application.get_env(:burn, :dns_cluster_query) || :ignore},
      {Phoenix.PubSub, name: Burn.PubSub},
      {Finch, name: Burn.Finch},
      {Registry, keys: :unique, name: Burn.Agents},
      {Phoenix.Sync.SandboxAwareSupervisor, children: [
        Burn.Agents.Supervisor
      ]}
      {BurnWeb.Endpoint, phoenix_sync: Phoenix.Sync.plug_opts()}
    ]

    opts = [strategy: :one_for_one, name: Burn.Supervisor]
    Supervisor.start_link(children, opts)
  end

  @impl true
  def config_change(changed, _new, removed) do
    BurnWeb.Endpoint.config_change(changed, removed)

    :ok
  end
end

I.e.: just nest processes that need to be sandbox aware inside Phoenix.Sync.SandboxAwareSupervisor and in :test it could wait until the sandbox has started before starting them. That way the app code is fairly unchanged and there's no need to duplicate the application tree manually in the test setup?

@thruflo
Copy link
Contributor

thruflo commented Jul 31, 2025

Pushed my latest status thruflo/burn#1 -- getting a bunch more errors running tests.

@magnetised
Copy link
Contributor Author

just nest processes that need to be sandbox aware inside Phoenix.Sync.SandboxAwareSupervisor and in :test it could wait until the sandbox has started before starting them. That way the app code is fairly unchanged and there's no need to duplicate the application tree manually in the test setup?

Interesting idea. I'm going to work on the materialised shape stuff then come back to this. I think it would be possible to make some sandbox-aware client state that worked with a sandbox in shared mode but don't want to open that up in this pr which has gone on too long already.

Copy link
Contributor

@thruflo thruflo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously would be nice to squish all warnings and connection errors in tests but let's merge and iterate if need be.

@magnetised magnetised merged commit d69b5da into main Aug 12, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Testability

3 participants